Fix Expression Formatter#217
Conversation
rcosta358
added
error messages
CatarinaGamboa
left a comment
CatarinaGamboa
left a comment
There was a problem hiding this comment.
I think we should keep the ite grouped expressions, maybe the --> as well. They would pass the parser correctly but its harder for developers to easily see the right associative rule - at least for me it is, what do you think?
| assertEquals("(a ? b : c) ? d : e", | ||
| new Ite(new GroupExpression(ite), new Var("d"), new Var("e")).toDisplayString()); | ||
| assertEquals("a ? b : (c ? d : e)", | ||
| assertEquals("a ? b : c ? d : e", |
There was a problem hiding this comment.
in here I think we want the parenthesis to still be there, right? reading it, it could either be
a ? b : c ? d : e
a) a ? b : (c ? d : e)
b) (a ? b : c) ? d : e
Also we can improve the tree construction as well, but you can do that in a followup PR
|
I think it's understandable without the parentheses but I get your point. |
CatarinaGamboa
left a comment
CatarinaGamboa
left a comment
There was a problem hiding this comment.
I see what you did with the examples since the last ones added () on purpose. But now its strange that some tests are just
assertEquals("x", parse("x").toDisplayString());
can we also add some with
assertEquals("x", parse("(x)").toDisplayString());
| assertEquals("#fresh¹²", new Var("#fresh_12").toDisplayString()); | ||
| assertEquals("#ret³", new Var("#ret_3").toDisplayString()); | ||
| assertEquals("this#Class", new Var("this#Class").toDisplayString()); | ||
| assertEquals("x", parse("x").toDisplayString()); |
There was a problem hiding this comment.
didnt you want to try assertEquals("x", parse("(x)").toDisplayString());
There was a problem hiding this comment.
Yep that case is missing, thanks.
2 participants
Description
This PR fixes the
ExpressionFormatterso grouped expressions only have parentheses when needed for precedence or associativity. This simplifies expressions in the error messages and in the context debugger.Example
Before and after:
(1)→1(a > 0)→a > 0a && (b > 0)→a && b > 0Unchanged cases:
a - (b + c)x == (1 > 0)!(x > 0)Related Issue
None.
Type of change
Checklist
ExpressionFormatterTestmvn testpasses locally